Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ テーブル ] 縦積み機能を追加 #2410

Merged
merged 19 commits into from
Jan 30, 2025

Conversation

mtdkei
Copy link
Contributor

@mtdkei mtdkei commented Jan 21, 2025

チケットへのリンク / 変更の理由(元のissueがあればリンクを貼り付ければOK)

#2406

どういう変更をしたか?

コアのテーブルブロックで縦積みにする機能を追加しました。
水平方向スクロール機能のように「縦積みのブレイクポイント」が選べるようになっています。
また、これに合わせてVKアイコンの色がパネルごとに変更されるようにしました。

スクリーンショットまたは動画

変更前 Before

変更後 After

2025-01-22.8.20.49.mov

実装者の確認事項

実装者はレビュワーに回す前に以下の事を確認してチェックをつけてください。

  • 複数の意図の変更 ( 機能の不具合修正 + 別の機能追加など ) を含んでいないか?
  • Files changed (変更ファイル)の内容は目視で確認したか?
  • readme.txt に変更内容は書いたか?
  • readme.txt に記載の変更内容はエンドユーザーが見て変更の概要がわかるように書かれているか?
  • 本当にちゃんと確認をしたか?

プログラムの変更の場合

  • 書けそうなテストは書いたか? → coreのextentionにはテストが存在しないためスキップ。必要なら書いてみます。

変更内容について何を確認したか、どういう方法で確認をしたかなど

  1. feature/table/align-vertical のブランチにし、npm run build をする。
    (npm run build:cache や npm run build:dev だとscssがビルドされないことがあり、分割読み込みをONにすると追加したcssが効かなくなることがあるため。)
  2. 編集画面でカラムの多いテーブルを作成。
    以下のHTML参照:
<!-- wp:table {"className":"is-style-vk-table-border-stripes","scrollable":false,"scrollBreakpoint":"table-scrollable-pc"} -->
<figure class="wp-block-table is-style-vk-table-border-stripes"><table><tbody><tr><td>aaaaa</td><td>11111111</td><td>おおおおおおおお</td><td>テストテスト</td><td>ああああああ</td><td>11111111</td><td>漢字が入るよ</td><td>ああああああああああああ</td></tr><tr><td></td><td></td><td></td><td></td><td></td><td></td><td></td><td></td></tr></tbody></table></figure>
<!-- /wp:table -->
  1. Vertical AlignmentをONにし保存。この時に編集画面で横スクロールができたことを確認。
  2. フロントエンドで縦積みになることを確認。(Mobileは575.98px以下の画面、Tabletは991.98px以下の画面、PCは全ての画面サイズ)

また、以下を確認しました。

  • Vertical AlignmentをONにしてその後OFFにした時、縦積みが外れることを編集画面とフロントエンドで確認。
  • 横スクロールと組み合わせた時、お互いの設定が生きていることを確認。
  • 既存のテーブルブロックを含む編集画面を開いた時、テーブルブロックに「このブロックには、想定されていないか無効なコンテンツが含まれています。」が表示されないことを確認。
  • 分割読み込みをON/OFFにして確認。

レビュワーに回す前の確認事項

  • 実装者はこのテンプレートのチェック項目をちゃんと確認してチェックしたか?

レビュワー確認方法・確認内容など

実装者と同じ確認を行なってください。
開発の方はコードの確認もお願いいたします。


レビュワー向け

レビュワーが確認して変更が反映されていない場合の確認事項

レビューしてみて意図した動作をしない場合は再度ビルドするなど以下の項目を確認してください。

  • プルしたか?
  • ビルドしたか?
  • ビルドしたディレクトリは正しいか(別の開発環境のディレクトリを見ていないか)?
  • npm install したか?
  • composer install したか?
  • キャッシュをクリアして確認したか?

@mtdkei mtdkei changed the title [ テーブル ] 縦積み機能を追加 【確認待ち】[ テーブル ] 縦積み機能を追加 Jan 21, 2025
@mtdkei mtdkei self-assigned this Jan 21, 2025
@mtdkei mtdkei marked this pull request as ready for review January 22, 2025 00:21
@sysbird sysbird changed the title 【確認待ち】[ テーブル ] 縦積み機能を追加 【確認中】[ テーブル ] 縦積み機能を追加 Jan 28, 2025
@sysbird sysbird changed the title 【確認中】[ テーブル ] 縦積み機能を追加 【2人め確認まち】[ テーブル ] 縦積み機能を追加 Jan 28, 2025
@sysbird
Copy link
Member

sysbird commented Jan 28, 2025

@mtdkei
いいですね!
table が使いやすくなります
ブレークポイントを選べるのもよいし、縦積みになったときに border が増える(→ tr の箇所が太く見える)のも気が利いてると思いました

2人め確認をどなたかお願いします

@kurudrive kurudrive changed the title 【2人め確認まち】[ テーブル ] 縦積み機能を追加 【2人め確認中】[ テーブル ] 縦積み機能を追加 Jan 28, 2025
Comment on lines 134 to 148
&[data-align-vertical-breakpoint="table-align-vertical-mobile"] {
@media (max-width: 575.98px) {
@include align-vertical-table;
}
}

&[data-align-vertical-breakpoint="table-align-vertical-tablet"] {
@media (max-width: 991.98px) {
@include align-vertical-table;
}
}

&[data-align-vertical-breakpoint="table-align-vertical-pc"] {
@include align-vertical-table;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtdkei

&[data-align-vertical-breakpoint="table-align-vertical-mobile"] {

これだと align-vertical がダブってて冗長だから

&[data-table-vertical-breakpoint="mobile"] {

でいいんじゃないかと思うけどどう?
あと、個人的には align は要素が左右だったり上下に”揃う(寄せる)”イメージなので、vertical-breakpoint だけで"縦積み"は伝わると思うけどどうかな?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtdkei と...思ったけど スクロールの value が table-scrollable-mobile だからあわせた方がいいのか...

じゃ、

&[data-vertical-breakpoint="table-vertical-mobile"] {

でどうでしょう?

え?「ちっさ! align 入ってるくらいええやろ!」って?
うん...まぁ...うん...そんな気もする...けど...ほら...先述の通り align だとテーブルの中の要素の上下揃えかなというイメージで "縦積み" とはニュアンスが違うかなと感じたのですがどうでしょう...(汗

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtdkei

それか &[data-cell-align-vertical-breakpoint="table-cell-align-vertical-mobile"] { か...

あ、そうだ

&[data-cell-vertical-breakpoint="table-cell-vertical-mobile"] {

でいいんじゃない? align のところ全部 cell に書き換えないとあかんけど...

ど...どどど...どうですかね?
※反対意見歓迎

Copy link
Contributor Author

@mtdkei mtdkei Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kurudrive
色々考えていただきありがとうございます。

&[data-cell-vertical-breakpoint="table-cell-vertical-mobile"] {

が良さそうですね!書き換えてみます。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kurudrive
関数名、ラベル名を調整しました。

table.removeAttribute('data-align-vertical-breakpoint');
} else {
// alignVertical が ON の場合
table.classList.add('is-style-vk-table-align-vertical');
Copy link
Member

@kurudrive kurudrive Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtdkei ありがとうございます。
VK Block Patterns の .vk-table--mobile-block とバッティングする(パターンやデモサイトデータなど、何気に使われてるサイトがかなり多い)ので、
on の場合は vk-table--mobile-block も存在してたら削除した方が無難かなと思います。

@mtdkei mtdkei changed the title 【2人め確認中】[ テーブル ] 縦積み機能を追加 【2人め確認中 / 修正中】[ テーブル ] 縦積み機能を追加 Jan 29, 2025
@mtdkei mtdkei changed the title 【2人め確認中 / 修正中】[ テーブル ] 縦積み機能を追加 【2人め確認中】[ テーブル ] 縦積み機能を追加 Jan 29, 2025
@kurudrive
Copy link
Member

kurudrive commented Jan 29, 2025

@mtdkei 変更ありがとうございます!
編集画面で画面サイズでの縦積みが効かないっぽいけど、気のせい?

※ もちろん明日以降で可

@mtdkei
Copy link
Contributor Author

mtdkei commented Jan 30, 2025

@kurudrive
ありがとうございます。編集画面での挙動を修正しました。
横スクロールも対応できてなかったので迷いましたがblockPropsに1行追加して対応しています。もし別のプルリクが良さそうでしたらそういたします。

@kurudrive
Copy link
Member

@mtdkei 編集画面の対応確認いたしました。ありがとうございます!

↓の件も対応よろしくお願いいたします ( ̄人 ̄)
#2410 (comment)

@mtdkei
Copy link
Contributor Author

mtdkei commented Jan 30, 2025

@kurudrive
すみません、見逃しておりました。ただいま修正しました。

@kurudrive
Copy link
Member

@mtdkei ありがとうございました!

@kurudrive kurudrive changed the title 【2人め確認中】[ テーブル ] 縦積み機能を追加 [ テーブル ] 縦積み機能を追加 Jan 30, 2025
@kurudrive kurudrive merged commit 386389e into develop Jan 30, 2025
14 checks passed
@kurudrive kurudrive deleted the feature/table/align-vertical branch January 30, 2025 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants